Skip to content

Conversation

@samgst-amazon
Copy link
Contributor

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
    Implement polling service to check for new notifications from a remote endpoint

Description

Checklist

  • My code follows the code style of this project
  • I have added tests to cover my changes
  • A short description of the change has been added to the CHANGELOG if the change is customer-facing in the IDE.
  • I have added metrics for my changes (if required)

License

I confirm that my contribution is made under the terms of the Apache 2.0 license.

Comment on lines 126 to 158
/**
* Loads notifications, preferring the latest downloaded version if available,
* falling back to bundled resource if no downloaded version exists
*/
private fun loadLocalNotifications(): NotificationFile? {
// First try to load from system directory (latest downloaded version)
getLocalNotificationsPath().let { path ->
if (path.exists()) {
try {
val content = path.readText()
return deserializeNotifications(content)
} catch (e: Exception) {
LOG.error(e) { "Failed to load downloaded notifications, falling back to bundled resource" }
}
}
}

// Fall back to bundled resource if no downloaded version exists
return try {
val resourceStream = javaClass.getResourceAsStream(NOTIFICATIONS_RESOURCE_PATH)
?: return null

val content = resourceStream.use { stream ->
stream.bufferedReader().readText()
}

deserializeNotifications(content)
} catch (e: Exception) {
LOG.error(e) { "Failed to load notifications from bundled resources" }
null
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isnt this just the RemoteResource system

@github-actions
Copy link

github-actions bot commented Nov 20, 2024

Qodana Community for JVM

18 new problems were found

Inspection name Severity Problems
Unused symbol 🔶 Warning 5
Unused import directive 🔶 Warning 3
Extension class should be final and non-public 🔶 Warning 2
Constant conditions 🔶 Warning 2
Class member can have 'private' visibility ◽️ Notice 4
Cascade 'if' can be replaced with 'when' ◽️ Notice 1
Function or property has platform type ◽️ Notice 1

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at [email protected]

import com.intellij.openapi.project.Project
import com.intellij.openapi.startup.ProjectActivity

class NotificationServiceInitializer : ProjectActivity {

Check warning

Code scanning / QDJVMC

Extension class should be final and non-public Warning

Extension class should not be public
@manodnyab manodnyab changed the base branch from main to feature/ideNotifs November 20, 2024 22:27
@manodnyab manodnyab changed the base branch from feature/ideNotifs to main November 20, 2024 22:28
@samgst-amazon samgst-amazon changed the base branch from main to feature/ideNotifs November 20, 2024 22:36
}

@Service(Service.Level.APP)
class NotificationPollingServiceImpl :

Check warning

Code scanning / QDJVMC

Extension class should be final and non-public Warning

Service implementation should not be public. If a service is supposed to be used outside its module, extract an interface from it and specify it as serviceInterface in plugin.xml.
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QDJVMC found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.


fun processNotification() {
// TODO: calls the Rule engine and notifies listeners
fun processNotification(project: Project, notificationData: NotificationData) {

Check warning

Code scanning / QDJVMC

Unused symbol Warning

Function "processNotification" is never used
* Emits telemetry metric for polling failures
*/
private fun emitFailureMetric(exception: Exception?) {
// todo: add metric
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this already exists in common, we can add this metric

class NotificationServiceInitializer : ProjectActivity {

override suspend fun execute(project: Project) {
val service = ApplicationManager.getApplication().getService(NotificationPollingService::class.java)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use the getInstance method here?


override suspend fun execute(project: Project) {
val service = ApplicationManager.getApplication().getService(NotificationPollingService::class.java)
RunOnceUtil.runOnceForApp(this::class.qualifiedName.toString()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this? Wont the startup activity anyway run just once?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to explicitly only run once per IDE instance and not per-project. It's what I understood to work, don't know about a better way yet

* Main polling function that checks for updates and downloads if necessary
* Returns the parsed notifications if successful, null otherwise
*/
private fun pollForNotifications(): Boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add tests for this?

andrewyuq and others added 2 commits November 21, 2024 12:49
1. Auto-trigger now triggers much more frequently and suggestions will consistently show whenever user stops typing.
The key shortcuts for Q inline suggestions are now configurable from the keymap settings. Default key shortcut for force accept is option + tab or option + enter and default for navigation keys has now changed from <- (navigating to prev) and -> (navigating to next) arrow keys to option + [ and option + ] , respectively.
2. Amazon Q suggestions can now co-exist with JetBrains code completions (IntelliSense). When both suggestions appear on the screen, tab accepts JetBrains suggestions first. Users will also have other key shortcuts to force accept the other Q suggestion (option + tab or option + enter)
3. The Amazon Q suggestion popup is now hidden by default and only appears above the current editing line when the user hovers over the suggestion preview text. IntelliSense popup (if it's showing) will also appear to be more transparent when user hovers over the suggestion preview.
startTime = endTime
val requestId = response.responseMetadata().requestId()
val sessionId = response.sdkHttpResponse().headers().getOrDefault(KET_SESSION_ID, listOf(requestId))[0]
if (requestCount == 1) {

Check warning

Code scanning / QDJVMC

Constant conditions Warning

Condition 'requestCount == 1' is always true
LOG.debug { "Skipping sending remaining requests on inactive CodeWhisperer session exit" }
return
}
if (requestCount >= PAGINATION_REQUEST_COUNT_ALLOWED) {

Check warning

Code scanning / QDJVMC

Constant conditions Warning

Condition 'requestCount >= PAGINATION_REQUEST_COUNT_ALLOWED' is always true
content = message("codewhisperer.notification.custom.not_available"),
project = requestContext.project,
notificationActions = listOf(
NotificationAction.create(

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
message("codewhisperer.notification.custom.simple.button.select_another_customization")
) { _, notification ->
CodeWhispererModelConfigurator.getInstance().showConfigDialog(requestContext.project)
notification.expire()

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
displayMessage = e.awsErrorDetails().errorMessage() ?: message("codewhisperer.trigger.error.server_side")
} else if (e is CodeWhispererRuntimeException) {
requestId = e.requestId().orEmpty()
sessionId = e.awsErrorDetails().sdkHttpResponse().headers().getOrDefault(KET_SESSION_ID, listOf(requestId))[0]

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
displayMessage = e.awsErrorDetails().errorMessage() ?: message("codewhisperer.trigger.error.server_side")
} else {
requestId = ""
sessionId = ""

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
message("codewhisperer.trigger.error.server_side")
} else {
message("codewhisperer.trigger.error.client_side")
}

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'message(String, vararg Any): String' is deprecated. Use extension-specific localization bundle instead
}
(newLookup as LookupImpl).component.addMouseHoverListener(
newLookup,
object : HoverListener() {

Check warning

Code scanning / QDJVMC

Unstable API Usage Warning

'com.intellij.ui.hover.HoverListener' is marked unstable with @ApiStatus.Experimental
}
(newLookup as LookupImpl).component.addMouseHoverListener(
newLookup,
object : HoverListener() {

Check warning

Code scanning / QDJVMC

Unstable API Usage Warning

'HoverListener()' is declared in unstable class 'com.intellij.ui.hover.HoverListener' marked with @ApiStatus.Experimental
(newLookup as LookupImpl).component.addMouseHoverListener(
newLookup,
object : HoverListener() {
override fun mouseEntered(component: Component, x: Int, y: Int) {

Check warning

Code scanning / QDJVMC

Unstable API Usage Warning

Overridden method 'mouseEntered([email protected] Component, int, int)' is declared in unstable class 'com.intellij.ui.hover.HoverListener' marked with @ApiStatus.Experimental
)
}

override fun mouseMoved(component: Component, x: Int, y: Int) {}

Check warning

Code scanning / QDJVMC

Unstable API Usage Warning

Overridden method 'mouseMoved([email protected] Component, int, int)' is declared in unstable class 'com.intellij.ui.hover.HoverListener' marked with @ApiStatus.Experimental
}

override fun mouseMoved(component: Component, x: Int, y: Int) {}
override fun mouseExited(component: Component) {}

Check warning

Code scanning / QDJVMC

Unstable API Usage Warning

Overridden method 'mouseExited([email protected] Component)' is declared in unstable class 'com.intellij.ui.hover.HoverListener' marked with @ApiStatus.Experimental
}

// Helper method to get Path from stored String
fun getCachedPath(): Path? =

Check notice

Code scanning / QDJVMC

Class member can have 'private' visibility Note

Function 'getCachedPath' could be private

package software.aws.toolkits.jetbrains.core.notifications

import com.fasterxml.jackson.databind.DeserializationFeature

Check warning

Code scanning / QDJVMC

Unused import directive Warning

Unused import directive
package software.aws.toolkits.jetbrains.core.notifications

import com.fasterxml.jackson.databind.DeserializationFeature
import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper

Check warning

Code scanning / QDJVMC

Unused import directive Warning

Unused import directive
import com.fasterxml.jackson.module.kotlin.readValue
import com.intellij.openapi.project.Project
import software.aws.toolkits.core.utils.inputStream
import java.io.InputStream

Check warning

Code scanning / QDJVMC

Unused import directive Warning

Unused import directive
import java.nio.file.Path

object NotificationMapperUtil{
val mapper = jacksonObjectMapper().configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)

Check notice

Code scanning / QDJVMC

Function or property has platform type Note

Declaration has type inferred from a platform call, which can lead to unchecked nullability issues. Specify type explicitly as nullable or non-nullable.
fun getNotificationsFromFile() {
// TODO: returns a notification list
}
fun getNotificationsFromFile(path: Path): NotificationsList =

Check notice

Code scanning / QDJVMC

Class member can have 'private' visibility

Function 'getNotificationsFromFile' could be private
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants